Fail deferred Cloud Composer tasks when the GCP operation errors - #70430
Conversation
boring-cyborg
Bot
added
area:providers
provider:google
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Good catch, and the diagnosis is exactly right. Because the long-running operation contract guarantees that neither error nor response is populated while done is false, the elif operation.error.message branch could never be reached — a failed operation arrives as done=True with error set, took the first branch, broke out of the loop, and the trigger then yielded
TriggerEvent({"operation_name": ..., "operation_done": True})which the operator reads as success. So a failed environment create/delete/update reported success rather than failing the task. Silent false success is about the worst failure mode a deferrable operator can have.
Nesting the check inside the done branch is the right fix, since that is the only state in which error can be set.
The tests are well chosen: test_run_raises_when_operation_finished_with_error genuinely fails against the old code — it would yield the success event instead of raising — and the companion test pins the happy path so the fix cannot over-fire. Putting the LRO contract in make_operation's docstring is a nice touch; it puts the reasoning where the next person will look.
I rebased the branch onto main before merging, as it had drifted a long way behind.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
30709e3 to
412de74
Compare
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
2 participants
CloudComposerExecutionTrigger.run()checkedoperation.donebeforeoperation.error, in anif/elif. Per the long-running operation contract, "ifdone==false, neithererrornorresponseis set", and a finished operation hasdone=Truewith exactly one oferrororresponsepopulated.So the two branches were inverted relative to the API:
elif operation.error.messagebranch was only ever reached whiledonewasfalse— the one state whereerroris guaranteed unset, making it dead code;done=Truewitherrorset) broke out of the loop and yieldedoperation_done: True, the success shape.The consequence differs per operator, all three of which defer with this trigger:
execute_completeCloudComposerUpdateEnvironmentOperatoroperation_done, no state checkCloudComposerDeleteEnvironmentOperatorpassCloudComposerCreateEnvironmentOperatorNotFound/wrong-state error rather than the GCP failure reasonThis also made the deferrable path disagree with the synchronous one: with
deferrable=Falsethe operators callhook.wait_for_operation()→operation.result(), which raises on a failed operation.Checking
errorinside thedonebranch fixes all three. The existingraiseis relocated, not new.CloudComposerExecutionTriggerhad no test coverage, so this adds two cases: a finished-with-error operation must raise, and a finished-without-error operation must still yieldoperation_done: True. The first fails without this change; the second guards the new nested condition against over-raising on success.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5) following the guidelines